feat(triage): effort-estimation skill and auto-promotion gate - #646
feat(triage): effort-estimation skill and auto-promotion gate#646rh-hemartin wants to merge 1 commit into
Conversation
|
🤖 Finished Review · ✅ Success · Started 1:05 PM UTC · Completed 1:23 PM UTC |
PR Summary by QodoAdd effort-estimation gating and block_auto_promotion to triage auto-promotion
AI Description
Diagram
High-Level Assessment
Files changed (30)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsHigh
Medium
Low
Labels: PR modifies triage agent instructions, post-script, skill, and eval cases. Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b672253 to
3d72bb1
Compare
|
🤖 Review · |
3d72bb1 to
2686001
Compare
|
🤖 Review · |
2686001 to
743fabd
Compare
|
🤖 Review · |
743fabd to
f8405d6
Compare
|
🤖 Finished Review · ✅ Success · Started 3:00 PM UTC · Completed 3:16 PM UTC |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
[MEDIUM] Effort-estimation threshold and traceability are unverified before merge (PR description / skills/effort-estimation/SKILL.md)
Three compounding gaps:
-
The PR body says "Closes #2207", but
repos/fullsend-ai/agents/issues/2207returns 404 — issue #2207 does not exist in this repo. The actual issue isfullsend-ai/fullsend#2207("dispatch: no effort-based gating before auto-promoting issues to coder"), which is open. GitHub's auto-close syntax only closes same-repo issues unless theowner/repo#numberform is used, so merging as-is will not close the tracked issue, and the entire feature (the four scoring dimensions, 1-5 scale, and the specific ">=4 blocks" threshold) is only traceable to a cross-repo issue that the PR text doesn't correctly reference. -
There is no cited derivation (historical data, prior incidents) for the ">=4 average blocks" threshold in
skills/effort-estimation/SKILL.md; it reads as a picked-because-it-feels-right number, and no eval case near the boundary (e.g. ~3.5-4.5) exists to confirm the cutoff behaves as intended — the two added fixtures (eval/triage/cases/008, 009) are deliberately constructed extremes. -
The PR's own test plan leaves unchecked: "Eval run confirms case 008 gets
triagedand 009 getsready-to-code" — meaning the one piece of evidence that would validate the threshold against a real triage agent run has not been collected, and this is shipped as final production gating logic rather than as an explicitly-flagged, tunable heuristic pending that verification.
Suggestion: update the PR description to "Closes fullsend-ai/fullsend#2207" so the cross-repo issue actually closes on merge. Before merging the gating logic as final, run the eval harness on cases 008/009 (and ideally a boundary case around effort ~4.0) and record actual pass/fail, or explicitly mark the threshold as a tunable default subject to recalibration pending that evidence.
|
About the compounding gaps: Fixed the PR body, the decision of using 1 to 5 has been raised in internal communications, not sure there is a reference and I don't think we need a middle case, it will bring flakyness, let's see how this goes; and the test plan has been checked. |
f8405d6 to
6423d80
Compare
|
🤖 Finished Review · ✅ Success · Started 6:32 AM UTC · Completed 6:51 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass (inline comments added below). One additional finding on unchanged code that isn't part of this PR's diff, so noting it here instead of inline:
[HIGH] Auto-promotion gate is undermined by the unconditional /fs-code footer appended to the same comment (scripts/post-triage.sh, the "Append action-hints footer (sufficient only)" block, ~line 558)
When block_auto_promotion.blocked is true and category is bug/documentation/performance, the script appends "Auto-promotion blocked: <reason>". But the unconditional "Next steps" footer, gated only on ACTION == "sufficient" with no check of AUTO_PROMOTION_BLOCKED, still appends "/fs-code — agent creates a PR to implement this issue". docs/code.md confirms /fs-code triggers the code agent independent of label state ("triggered when the ready-to-code label is applied... or via the /fs-code command"). The result is a single comment that both explains why the issue was held for review and hands the reader a working one-click bypass of that exact hold.
Suggested fix: Suppress or reword the Next-steps footer when AUTO_PROMOTION_BLOCKED is true, e.g. "This issue was held for review — run /fs-code only after confirming the concerns above."
| "recommended_fix": { "type": "string", "minLength": 1 }, | ||
| "proposed_test_case": { "type": "string", "minLength": 1 }, | ||
| "requires_workflow_changes": { "type": "boolean" } | ||
| "block_auto_promotion": { |
There was a problem hiding this comment.
[HIGH] Schema does not require block_auto_promotion for the categories the gate is meant to cover
block_auto_promotion is entirely optional inside $defs.triage_summary (not in the required array a few lines up), and unlike the top-level allOf block elsewhere in this schema which conditionally requires fields based on action, there is no conditional requiring block_auto_promotion when category is bug/documentation/performance. scripts/post-triage.sh reads it as jq -r '.triage_summary.block_auto_promotion.blocked // false', so if the model omits the field entirely (prompt drift, rushed run, or issue-content injection), the omission is indistinguishable from an explicit blocked: false and the issue silently auto-promotes to ready-to-code with no schema-validation error and no warning — exactly the scenario the gate exists to prevent. This is distinct from the existing "edge-case" thread on this object, which only discusses blocked/reason being required within the object once present, not the object's own presence being unenforced for the categories the gate targets.
Suggested fix: Add a conditional requirement (mirroring the existing top-level allOf pattern) inside $defs.triage_summary: when category is bug/documentation/performance, require block_auto_promotion to be present. This turns a silent gate bypass into a hard schema-validation error.
| @@ -0,0 +1,83 @@ | |||
| --- | |||
There was a problem hiding this comment.
[MEDIUM] New skill introduces a second, unreconciled "Effort" scoring scheme instead of reusing/aligning with the prioritize agent's existing RICE Effort dimension
fullsend-ai/fullsend#2207 (the issue this PR closes) explicitly names existing infrastructure to build on — both the issue body ("The prioritize agent already computes an Effort dimension via RICE") and the prioritize agent's own RICE comment on that issue ("Effort: 1 — The RICE prioritize agent already computes an Effort dimension, so the core estimation logic exists"). Instead, this skill defines an independent 4-dimension (scope/testing/domain/risk), 1-5, averaged rubric with a hardcoded >=4 block threshold. agents/prioritize.md shows the existing RICE Effort dimension uses a 0.25-3 scale with inverse semantics (higher effort = lower priority, used as a denominator). Neither the PR body, docs/triage.md, nor this file explains why the existing dimension wasn't reused or how the two same-named "Effort" concepts (different scales, different semantics, no cross-reference) are meant to coexist.
Suggested fix: Add a short note explaining why an independent scale was introduced (e.g., prioritize runs on a schedule, decoupled from synchronous triage), or align this rubric's scale/semantics with the prioritize agent's Effort definition.
| 4. High regression risk, needs careful rollout | ||
| 5. Breaking change affecting downstream consumers | ||
|
|
||
| Compute the overall effort as the average of the four dimensions, rounded |
There was a problem hiding this comment.
[MEDIUM] Averaging the four effort dimensions lets a single maximal-risk dimension slip through the gate undetected
Step 3 blocks only when the straight average of scope/testing/domain/risk is >= 4. An issue scoring Scope=1, Testing=1, Domain=1 but Risk=5 ("Breaking change affecting downstream consumers", the top of the Risk rubric above) averages to 2.0 and is NOT blocked — a change the rubric itself flags as breaking for downstream consumers would still auto-promote unattended. Both new eval fixtures (008 and 009) score uniformly across all four dimensions, so this dilution failure mode — the exact scenario the gate exists to catch — is completely untested by this PR's own eval suite.
Suggested fix: Use max() or a rule that any single dimension >= 4 forces blocking, rather than a straight average. Add an eval case with a mixed profile (one dimension at 5, others at 1) to confirm intended blocking behavior.
| auth middleware, rate_limit.py) — 4. | ||
| - Testing: no existing session lifecycle tests; needs new test | ||
| infrastructure for time-dependent behavior (mocking time.time) — 4. | ||
| - Domain knowledge: requires understanding session token security |
There was a problem hiding this comment.
[MEDIUM] Domain-knowledge annotation score contradicts the skill's own rubric, making this fixture potentially flaky at the >=4 threshold
This annotation scores "Domain knowledge... coordinating invalidation across the session store, auth middleware, and rate limiter" as a 4. But skills/effort-estimation/SKILL.md defines domain-knowledge level 4 as "Requires cross-repo or external API knowledge" and level 3 as "Requires understanding subsystem design". The described scenario (session store + auth middleware + rate limiter, all confirmed single-repo/in-process components per this fixture's own description) matches level 3, not 4. Recomputing with domain=3 instead of 4 (scope=4, testing=4, domain=3, risk=4) gives (4+4+3+4)/4 = 3.75, rounding to 3.8 per the skill's "rounded to one decimal place" rule — below the >=4 blocking threshold. A model faithfully applying the documented rubric could legitimately set blocked: false, contradicting this fixture's required triaged label and forbidden ready-to-code label.
Suggested fix: Either revise the justification to genuinely require cross-repo/external-API knowledge (matching level 4), or lower the expected score to 3 and re-verify the threshold math with the other three dimensions still yields >=4.
| ``` | ||
|
|
||
| **Workflow change detection (optional):** If the issue likely requires modifying GitHub Actions workflow files (`.github/workflows/`, `.fullsend/.github/workflows/`, or enrolled-repo shim workflows), set `requires_workflow_changes: true` in `triage_summary`. When set, the post-triage script skips auto-triggering the code agent because the code agent cannot modify workflow files under current permissions. The triage comment should warn about this limitation and note that manual intervention is required. When `requires_workflow_changes` is not set or is `false`, auto-triggering proceeds normally. | ||
| **Blocking auto-promotion:** Use the `block_auto_promotion` field in `triage_summary` to prevent the post-triage script from auto-promoting the issue to the code agent. Set `blocked: true` with a `reason` when: |
There was a problem hiding this comment.
[MEDIUM] "Blocking auto-promotion" section overstates block_auto_promotion's effect versus what post-triage.sh actually implements
This line tells the model it can set blocked: true for "any other condition where auto-dispatch would be premature" (not just effort/workflow-changes) and states unqualified that "the post-script applies triaged instead of ready-to-code and appends the reason to the triage comment." In reality, scripts/post-triage.sh only honors block_auto_promotion inside a gate requiring CATEGORY to be bug/documentation/performance — for feature, security, or other categories the flag is silently ignored and the "Auto-promotion blocked:" footer is never appended. A maintainer or future contributor reading only this section would reasonably expect the reason to always surface when blocked: true, which is false for three of six category values.
Suggested fix: Qualify the doc to state explicitly that the reason-append/triaged-override behavior only applies to bug/documentation/performance categories, matching the CATEGORY guard in post-triage.sh.
Add an effort-estimation skill that scores issues on four dimensions
(1-5 each). An average >= 4 blocks auto-promotion, keeping the issue
at triaged for human review.
Replace requires_workflow_changes with block_auto_promotion ({blocked,
reason}) in triage_summary and the post-script gate. Harden the
post-script against workflow-command injection.
Add eval cases 008 and 009 with runnable fixture repos.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Hector Martinez <hemartin@redhat.com>
6423d80 to
8a42a64
Compare
|
🤖 Finished Review · ✅ Success · Started 10:32 AM UTC · Completed 10:49 AM UTC Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional review findings (deduplicated against existing comments/reviews on this PR).
| to one decimal place. | ||
|
|
||
| ## Step 3: Derive the review decision | ||
|
|
There was a problem hiding this comment.
[HIGH] block_auto_promotion has two independent instruction sources with no explicit merge/OR rule between them
block_auto_promotion.blocked is populated once per triage run, but two separate pieces of guidance tell the model how to set it, and neither cross-references the other: (1) this skill's Step 3 (If the overall effort score is >= 4, the issue requires human review) and its Output section give two complete, self-contained example JSON blocks (blocked:true / blocked:false) driven purely by the effort score, with zero mention of workflow-file changes; (2) agents/triage.md Phase 3 separately instructs detecting CI/workflow-file changes and setting block_auto_promotion.blocked=true for that reason. Only agents/triage.md's later "Blocking auto-promotion" bullet list explicitly OR-combines all conditions ("effort-estimation... OR CI/pipeline files... OR any other condition"), so a model that reads the full prompt attentively could reconcile them — but this skill, whose own worked examples show a complete blocked:false JSON snippet keyed only on effort, gives the model a plausible, self-consistent template for silently overwriting an earlier workflow-file-driven true with false. post-triage.src.sh only reads the single final .triage_summary.block_auto_promotion.blocked boolean with no way to recover or cross-check a workflow-file signal once the field is written, so a low-effort fix to .github/workflows/ or .gitlab-ci.yml risks silently regressing the pre-existing #325 workflow-change guard this PR is supposed to preserve.
Suggested fix: Add an explicit cross-reference in this Step 3/Output section noting that blocked must remain true if any other condition (e.g. workflow-file changes) already requires it — i.e., make the skill instructions state the OR-combination itself rather than relying solely on the separate "Blocking auto-promotion" section elsewhere in triage.md. Consider also having post-triage.src.sh independently re-detect workflow-file changes (or track the two signals as separate fields, e.g. effort_blocked / workflow_blocked, ORed together in script logic) so the important workflow-file gate does not depend entirely on the model reconciling two documents correctly in one shot.
| COMMENT="${COMMENT} | ||
|
|
||
| --- | ||
| **Auto-promotion blocked:** ${BLOCK_REASON}" |
There was a problem hiding this comment.
[MEDIUM] block_auto_promotion.reason is appended to the comment after the fenced-code-block sanitization pass, so it can reintroduce fences unsanitized
The fenced-code stripping runs early, at lines 70-73, directly on COMMENT as read from the agent JSON (.comment). BLOCK_REASON (sourced separately from .triage_summary.block_auto_promotion.reason) is only sanitized for :: sequences (lines 393-397) and is then concatenated onto COMMENT much later, here at line 449 (**Auto-promotion blocked:** ${BLOCK_REASON}), after the fence-stripping pass has already run. If the model puts a triple-backtick fence inside block_auto_promotion.reason (a free-text field with no fence restriction documented anywhere), it survives verbatim into the final posted comment, bypassing the fence-stripping safeguard entirely. There is no code path that re-checks the fully-assembled comment after this append.
Suggested fix: Move the fenced-code-block check to run once against the fully-assembled comment (after this auto-promotion-blocked footer, and after the other append points elsewhere in the script), or explicitly sanitize BLOCK_REASON for fences before interpolating it, rather than only sanitizing the pre-footer COMMENT variable.
| triage_expectations: | | ||
| This issue reports a memory leak in the session store and a security | ||
| flaw where logout does not invalidate tokens. The fix touches at least | ||
| four files across three packages (auth, middleware, api) and requires |
There was a problem hiding this comment.
[MEDIUM] This new eval fixture has unresolved ambiguities that let it pass or fail without reliably exercising the effort gate
Three concrete issues: (1) this rubric tells the model to identify the issue as "a defect (bug or security)". If the model picks category security, post-triage.src.sh's category case (the *) default branch, since security isn't handled by the bug/documentation/performance/feature branches) always applies triaged and never applies ready-to-code, regardless of whether block_auto_promotion was ever set — so this fixture's required/forbidden label assertions (triaged required, ready-to-code forbidden) can pass trivially without the effort gate having fired at all. (2) The issue body describes three separable problems (session-eviction leak, no-op logout, rate-limiter leak accumulation) — the same "bundled independent concerns" pattern that agents/triage.md's own "Does this issue bundle multiple independent concerns?" guidance says should trigger action: split. post-triage.src.sh's split handler never applies the triaged label and instead closes the original issue — so if the model reasonably chooses split here, this fixture's required-label assertion (triaged) fails, making the outcome depend on a judgment call the prompt itself invites. (3) This line states the fix spans "three packages (auth, middleware, api)", but the repo fixture's src/api/users.py implements an unrelated mass-assignment vulnerability in update_user_handler that is never mentioned in the issue body and is not part of the actual described bug (session/logout/rate-limiter) — the rubric's claimed third package doesn't match any file the fix should actually touch.
Suggested fix: Tighten this fixture: require category: bug explicitly (drop the "or security" allowance, or add an explicit assertion that also constrains security's routing), add an explicit note that action: split should NOT be chosen here (or restructure the issue body so it reads as one cohesive defect rather than three independent bullet items), and correct the "three packages (auth, middleware, api)" claim to match the files actually in scope for the fix (drop the api/users.py reference or clarify it is an intentional red herring not counted toward scope).
2 participants
Summary
effort-estimationskill that scores implementation effort on a 1-5 scale across scope, testing, domain knowledge, and risk. Issues scoring >= 4 block auto-promotion.requires_workflow_changesboolean withblock_auto_promotionobject ({blocked, reason}) intriage_summary. The post-script gatesready-to-codevstriagedfor bug/documentation/performance categories.::warning::uses a fixed message (no interpolation of untrusted content), andBLOCK_REASONis sanitized (::collapsed to:) before any use.commentfield to prose-only (no fenced code blocks) to avoid model abbreviation of identifiers inside JSON strings.Migration: the schema replaces
requires_workflow_changeswithblock_auto_promotion. Becausetriage_summaryusesadditionalProperties: false, all components (schema, prompt, post-script) must be deployed and rolled back together per the lockstep release model.Closes fullsend-ai/fullsend#2207
Test plan
pytest+ server endpoints)triagedand 009 getsready-to-code🤖 Generated with Claude Code